Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency Graph UI/UX changes #10612

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Dependency Graph UI/UX changes #10612

merged 3 commits into from
Jul 16, 2024

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 16, 2024

https://www.loom.com/share/ce2d6b56d4914ee2afd81fa8a1bf2a9f

  • Split component stack and dependency graph into two accordion tabs (labeled "experimental")
  • Make tabs look and feel more like our Call Stack info (no "jump" button, just click the row to jump to code)
  • Auto-load dependency graph paths when accordion is expanded
  • Hide JSON viewer (this info is available in the Protocol panel)
  • Hide React-only dependency graph (the info was redundant)
Before After
Screenshot 2024-07-16 at 1 57 57 PM Screenshot 2024-07-16 at 1 58 16 PM

Copy link

vercel bot commented Jul 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
devtools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2024 7:49pm

Copy link

replay-io bot commented Jul 16, 2024

E2E Tests

Status In Progress ↗︎ 9 / 11
Commit c619bae
Results
99 Passed
  • async-stack: should detect async stacks outside the focus window
  • authenticated/comments-01: Test add, edit, and delete comment functionality
  • authenticated/comments-02: Test shared comments and replies (Replay 1, Replay 2)
  • authenticated/comments-03: Comment previews
  • authenticated/logpoints-01: Shared logpoints functionality (Replay 1, Replay 2)
  • authenticated/passport-01: Time travel
  • authenticated/passport-02: Infrared inspection
  • authenticated/passport-03: Swiss army knife
  • authenticated/passport-04: Multiplayer
  • console_async: support console evaluations in async frames
  • console_dock: Should show the correct docking behavior for recordings with video
  • console_errors: Test that errors and warnings from various sources are shown in the console
  • console_eval: support console evaluations
  • console_warp-01: should support warping to console messages
  • console_warp-02: support pausing, warping, stepping and evaluating console messages
  • console-expressions-01: should cache input eager eval and terminal expressions per instance
  • cypress-01: Basic Test Suites panel functionality
  • cypress-02: Test Step timeline behavior
  • cypress-03: Test Step interactions
  • cypress-04: Test Step buttons and menu item
  • cypress-05: Test DOM node preview on user action step hover
  • deleted-recording: Show error message for deleted recording
  • elements-search: Element panel should support basic and advanced search modes
  • fe-1875 :: verify that steps go to the right point in time
  • file-search-01: should search files
  • focus_mode-01: should filter messages as regions based on the active focus mode
  • highlighter: element highlighter works everywhere
  • inspector-computed-01: Basic computed styles can be viewed
  • inspector-computed-02: Complex computed styles can be viewed
  • inspector-elements-01: Basic DOM tree node display
  • inspector-elements-02_node-picker: element picker and iframe behavior
  • inspector-elements-03: Nested node picker and selection behavior
  • inspector-elements-04: Keyboard shortcuts should select the right DOM nodes
  • inspector-elements-05_search: element picker and iframe behavior
  • inspector-rules-01: Basic CSS rules should be viewed
  • inspector-rules-03: Shorthand CSS rules should be viewed
  • jump-to-code-01: Test basic jumping functionality
  • logpoints-01: log-points appear in the correct order and allow time warping
  • logpoints-02: conditional log-points
  • logpoints-03: should display event properties in the console
  • logpoints-04: should display exceptions in the console
  • logpoints-05: should auto-complete based on log point location
  • logpoints-06: should be temporarily disabled
  • logpoints-07: should use the correct scope in auto-complete
  • logpoints-08: should support jumping directly to a hit point via the capsule input
  • logpoints-09: should support pending edits
  • logpoints-10: too-many-points-to-find UX
  • logpoints-11: too-many-points-to-run-analysis UX
  • logpoints-12: should auto save when removing conditions
  • logpoints-13: Test log point in a sourcemapped file
  • network-01: should filter requests by type and text
  • network-02: should show details for the selected request
  • network-03: should sync and display the current time in relation to the network requests
  • node_console_dock: Should show the correct docking behavior for recordings without video
  • node_console-01: Basic node console behavior
  • node_console-02: uncaught exceptions should show up
  • node_logpoint-01: Basic node logpoints
  • node_object_preview: Showing console objects in node
  • node_quick_open_modal-01: Test basic searching functionality
  • node_spawn: Basic subprocess spawning
  • node_stepping-01: Test stepping in async frames and async call stacks
  • node_worker-01: make sure node workers don't cause crashes
  • object_preview-01: expressions in the console after time warping
  • object_preview-02: should allow objects in scope to be inspected
  • object_preview-03: Test previews when switching between frames and stepping
  • object_preview-04: Test scope mapping and switching between generated/original sources
  • object_preview-05: Should support logging objects as values
  • object_preview-06: HTML elements
  • object_preview-07: inspect objects in the console while paused somewhere else
  • object_preview-08: should render ellipsis for collapsed objects with truncated properties
  • playwright-01: Basic Test Suites panel functionality
  • playwright-02: Test Step timeline behavior
  • playwright-03: Test Step interactions
  • playwright-04: Test Step buttons and menu item
  • playwright-05: Test DOM node previews on user action step hover
  • react_devtools-01: Basic RDT behavior
  • react_devtools-02: RDT integrations (Chromium)
  • react_devtools-03: process and display multiple React versions in page
  • react_devtools-04: Component selection is maintained when seeking to a new point
  • redux_devtools: Test Redux DevTools.
  • repaint-01: repaints the screen screen when stepping over code that modifies the DOM
  • repaint-02: repaints on hover
  • repaint-03: repaints on seek
  • repaint-04: prefers nearest (<=) paint when seeking between paints
  • repaint-05: prefers current time if pause creation failed outside of the focus window
  • repaint-06: repaints the screen screen when stepping over code that modifies the DOM
  • resizable-panels-01: Left side Toolbar and Video should be collapsible
  • restart-session: restart debugging session
  • scopes_rerender: Test that scopes are rerendered
  • session-destroyed: errors caused by session failure should bubble to the root
  • source-line-highlights: Test source line highlighting
  • sourcemap_stacktrace: Test that stacktraces are sourcemapped
  • stacking: Element highlighter selects the correct element when they overlap
  • stepping-01: Test basic step-over/back functionality
  • stepping-02: Test fixes for some simple stepping bugs
  • stepping-04: Test stepping in a frame other than the top frame
  • stepping-05: Test stepping in pretty-printed code
  • stepping-06: Test stepping in async frames and async call stacks
  • stepping-07: Test quick stepping using the keyboard
  • @bvaughn bvaughn requested review from markerikson and removed request for jcmorrow July 16, 2024 18:07
    @markerikson
    Copy link
    Collaborator

    I like the changes in general, but have some thoughts / concerns:

    • Like with call stacks, I think there needs to be a separation between "the point we loaded the data for" and "where we're currently paused". What's trickier in this case is that with call stacks, we jump you to the other frame's line of code, but don't change the pause point. With the "jump" buttons I've got here, that does change the pause point, but in my current mental / behavior model I want to retain the dep graph data for the original point. (Not sure exactly how you changed it yet, just riffing off the description.) I could see a potential argument for not directly exposing the "jump to that earlier point" button, similar to how we do with call stacks, and just selecting the line of code. I guess I'm trying to figure out how you actually would jump to the earlier point if all we do is show you the line, and with call stacks the answers are "add a print statement on the earlier line", or "right-click the frame and 'Jump to Frame' ".
    • There is different data that comes back for the "normal" Dep Graph vs the "React" dep graph, which was part of why I had both of those being displayed - to visualize the different data for the same point, and get a sense of what data is coming back.

    @bvaughn
    Copy link
    Contributor Author

    bvaughn commented Jul 16, 2024

    • There is different data that comes back for the "normal" Dep Graph vs the "React" dep graph, which was part of why I had both of those being displayed - to visualize the different data for the same point, and get a sense of what data is coming back.

    From my limited browsing, it seemed like the React path was always a subset of the "full" path. Is that not the case?

    @bvaughn
    Copy link
    Contributor Author

    bvaughn commented Jul 16, 2024

    • Like with call stacks, I think there needs to be a separation between "the point we loaded the data for" and "where we're currently paused".

    With this feature, we're always loading data from the "top frame" (the current point) which is why I have it highlighted as selected (matching our Call Stack UI)

    @markerikson
    Copy link
    Collaborator

    No, there were definitely differences in the results.

    In particular, try going to a component that rendered multiple times over the course of the app. Pick a hit point that was triggered by something like a local setState, then load the dep graph.

    The normal dep graph focuses on this one current event and how we got here. What you should see is that the "normal" dep graph shows, say, one entry that's Discrete Input click or something like that, whereas the "React" dep graph should show all of the ancestor component ReactElementCreated and ReactRendered entries, at the most recent times that those ancestor renders lead to this component - in other words, the "component stack" data, even though the ancestor components did not render as part of this render pass.

    @markerikson
    Copy link
    Collaborator

    and yeah, with the "jump" buttons, I was treating it slightly differently than call stacks - ie, going to both the earlier line and the point we hit that line. Definitely reasonable for either approach. I'm fine with the change here to match the similarity with call stack behavior.

    @bvaughn
    Copy link
    Contributor Author

    bvaughn commented Jul 16, 2024

    No, there were definitely differences in the results.

    Got it. I'll restore those separate stacks then.

    @bvaughn
    Copy link
    Contributor Author

    bvaughn commented Jul 16, 2024

    Interestingly sometimes the differences in these stacks look weird. For example, here

    I'm paused inside of a React component that's rendering:
    Screenshot 2024-07-16 at 3 42 56 PM

    But the React dep graph looks less helpful than the normal one:
    Screenshot 2024-07-16 at 3 42 50 PM

    @markerikson
    Copy link
    Collaborator

    markerikson commented Jul 16, 2024

    Yep, agreed, especially in that case. I've been showing bhackett cases like this and asking him to investigate, and he's made several improvements. (Who knows how much longer that will be feasible.)

    @bvaughn
    Copy link
    Contributor Author

    bvaughn commented Jul 16, 2024

    I guess the thing to do for now will be...I don't even know what. File Linear bugs with Replays attached? Will those even be looked at?

    @bvaughn bvaughn merged commit c0b288e into experimental Jul 16, 2024
    23 checks passed
    @bvaughn bvaughn deleted the experimental-ui branch July 16, 2024 19:55
    @bhackett1024
    Copy link
    Contributor

    I guess the thing to do for now will be...I don't even know what. File Linear bugs with Replays attached? Will those even be looked at?

    We need to discuss this at the company / leadership level. I'm happy to continue working on this but only if we can decide that it's important enough to pull attention away from the other work going on.

    @bvaughn
    Copy link
    Contributor Author

    bvaughn commented Jul 16, 2024

    I'm happy to continue working on this but only if we can decide that it's important enough to pull attention away from the other work going on.

    I was under the impression that the dependency graph feature was pretty central to possible Replay's AI work. Maybe the expectation is that this kind of work shouldn’t require any help from the TT team?

    @bhackett1024
    Copy link
    Contributor

    I was under the impression that the dependency graph feature was pretty central to possible Replay's AI work. Maybe the expectation is that this kind of work shouldn’t require any help from the TT team?

    OK, I synced with @jasonLaster to get caught up. We do want to continue this work, the plan we talked about is that @bvaughn go ahead and file linear issues around problems with the dependency graph and its react data. @markerikson can focus on fixes and I'll be available to support and help however I'm needed.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants